fix: Avoid DoesNotExist exception in TokenRefreshSerializer#861
Conversation
There was a problem hiding this comment.
Looks good to me, thank you for the contribution!
I added this PR on a checklist for next major release since it does some breaking changes: #871
| user = ( | ||
| get_user_model() | ||
| .objects.filter(**{api_settings.USER_ID_FIELD: user_id}) | ||
| .first() |
There was a problem hiding this comment.
this may not be expected since the USER_AUTHENTICATION_RULE expects a user object, not None
suggestion: simply check if the user exists OR check authentication rule does not pass rather than solely rely on the authentication rule
There was a problem hiding this comment.
Since the USER_AUTHENTICATION_RULE method already performs the user is not None check, would adding another check outside the method introduce unnecessary code duplication?
There was a problem hiding this comment.
yea, the problem is the initial typing was typed assuming the user object is valid...
I mean that's my fault for not checking that. I guess this is fine, but we should also update the typing for the authentication rule?
There was a problem hiding this comment.
Makes sense, updated
|
I didn't get as far as getting 500s, as I was upgrading from 5.0 and our unit tests failed on previously unexpected
|
Yes, it's a 500 error and a breaking change for me. That's why I opened the issue and submitted the patch right after v5.4.0 was released. |
|
Got it, it sounds like a patch release is necessary rather than a minor/major version upgrade. |
|
I'd say this sounds like a minor-release-level change, as it shouldn't require immediate code adjustments whether or not someone has implemented a workaround like my example in #860; the behavior will change slightly though. The exception thrown is the same as previously and is handled by DRF already so I wouldn't say it's a breaking change either. But of course the final decision is up to the maintainers - I'm just hoping the fix will land soon ;) |
4 participants
#860
For deleted users, they should be treated the same as when no active user is found. This
DoesNotExistexception was introduced in the previous version.